Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[16.0][IMP] maintenance_project: Remove the Project > Administrator permission dependency #424

Merged
merged 4 commits into from
Sep 23, 2024
Merged

[16.0][IMP] maintenance_project: Remove the Project > Administrator permission dependency #424

merged 4 commits into from
Sep 23, 2024

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Sep 13, 2024 •
edited
Loading

Changes done:

  • Remove the Project > Administrator permission dependency
  • Change create_project_from_equipment fields from equipments to 'Create project' button
  • Add groups="project.group_project_user" to project/task fields
  • Add groups="maintenance.group_equipment_manager" to maintenance fields

Please @pedrobaeza and @carlos-lopez-tecnativa can you review it?

@Tecnativa TT50829

@victoralmau
Copy link
Member Author

I have found one additional controversial thing, the project and task fields do not have groups set, i.e. a user without permissions in Projects can see those fields (even if they don't have a permissions error). Is it consistent that they can see them?

carlos-lopez-tecnativa
carlos-lopez-tecnativa reviewed Sep 13, 2024
View reviewed changes
Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally tested, but I have a question: isn't it necessary to have a migration script to unlink the group in existing databases?

dalonsod
dalonsod suggested changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@dalonsod dalonsod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment. And also, should access to project (and task) fields be restricted in the other views, not only for the form one, and for maintenance.request views too?

maintenance_project/views/maintenance_equipment_views.xml Show resolved Hide resolved
@dalonsod
Copy link
Contributor

I have found one additional controversial thing, the project and task fields do not have groups set, i.e. a user without permissions in Projects can see those fields (even if they don't have a permissions error). Is it consistent that they can see them?

@victoralmau looked at this, project addon adds a read ACL for every internal user called "project.project on partners" (and the same for tasks)... but task access in partner view is finally restricted to project users, it's something strange IMO. In fact, restricting access to projects (and tasks) in general for non-project users seems to be a good practice, I agree with it. But I've also looked at other OCA typical addon that adds project/task management (helpdesk_mgmt_project) and has the same behavior than current implementation of maintenance_project (there are no access restrictions!).

I should add such access restriction (not consistent, as you said), but not sure if this change could be controversial as well because a current functionality should be removed.

@pedrobaeza
Copy link
Member

@dalonsod real access is restricted by record rules, not ACLs.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Sep 13, 2024
@victoralmau victoralmau force-pushed the 16.0-imp-maintenance_project-TT50829 branch 5 times, most recently from bc23738 to 5b185e5 Compare September 16, 2024 08:00
@victoralmau victoralmau force-pushed the 16.0-imp-maintenance_project-TT50829 branch from 5b185e5 to 3bada6c Compare September 16, 2024 08:10
@victoralmau
Copy link
Member Author

Added extra changes and updated the PR description.

carlos-lopez-tecnativa
carlos-lopez-tecnativa approved these changes Sep 16, 2024
View reviewed changes
Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@victoralmau
Copy link
Member Author

IMO this is now ready to merge

@pedrobaeza
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-424-by-pedrobaeza-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3411c9c into OCA:16.0 Sep 23, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at f254804. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 16.0-imp-maintenance_project-TT50829 branch September 23, 2024 19:10
victoralmau added a commit to Tecnativa/maintenance that referenced this pull request Sep 24, 2024
@victoralmau
[OU-ADD] maintenance_project: Remove Project > Administrator from Equ…
d93a538 
…ipment manager group

Related to OCA#424

TT50829
This was referenced Sep 24, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@carlos-lopez-tecnativa carlos-lopez-tecnativa carlos-lopez-tecnativa approved these changes

@dalonsod dalonsod dalonsod approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
5 participants
@victoralmau @dalonsod @pedrobaeza @OCA-git-bot @carlos-lopez-tecnativa